kvserver: prevent finished proposal from being present in proposals map #94825

tbg · 2023-01-06T09:23:27Z

The conjecture in #86547 is that a finished proposal somehow makes its
way into the proposal map, most likely by never being removed prior to
being finished.

This commit adds an assertion that we're never outright inserting
a finished proposals, and better documents the places in which we're
running a risk of violating the invariant.

It also clarifies the handling of proposals in an apply batch when a
replication change that removes the replica is encountered. I suspected
that this could lead to a case in which proposals would be finished
despite remaining in the proposals map. Upon inspection this turned out
to be incorrect - the map (at least by code inspection) is empty at that
point, so the invariant holds trivially.

Unfortunately, that leaves me without an explanation for #86547, but the
newly added invariants may prove helpful.

Touches #86547.

Epic: None
Release note: None

cockroach-teamcity · 2023-01-06T09:23:35Z

This change is

erikgrinaker · 2023-01-09T14:41:14Z

I'm not a huge fan of these fatal assertions, since they can be far worse than the problem they try to prevent when hit in production. Can we come up with a scheme for assertions that are compiled out in production builds?

tbg · 2023-01-10T07:39:20Z

Can we come up with a scheme for assertions that are compiled out in production builds?

Fair point. We can

if buildutil.CrdbTestBuild && (...) {

}

everywhere, it's a bit difficult to have generic helpers in Go because that usually costs you in allocations, etc.

I filed #94979 to track this, we should do a proper pass for stability.

tbg · 2023-01-10T07:41:22Z

Put the assertion behind the build tag in this PR.

pav-kv · 2023-01-10T08:06:52Z

if buildutil.CrdbTestBuild && (...) {

}

Alternatively, could do something like:

if (...) {
	buildutil.AlmostFatalf("something happened")
}

...

func AlmostFatalf(...) {
	if CrdbTestBuild {
		log.Fatalf(...)
	}
	log.Errorf(...)
}

or:

if buildutil.CrdbTestBuild {
	assert.Equal(a, b) // will these be optimized out if CrdbTestBuild == false?
	assert.True(c)
}

...

package assert // something akin to testify assert

tbg · 2023-01-10T10:00:50Z

I think a require-style API for runtime assertions would be useful (cc @cockroachdb/test-eng). Ideally, we wouldn't need to gate everything behind the build tag explicitly.
Anyway, this is a bit out of scope for this PR - we really need test-eng to be involved here. Note issue #86678 which indicates that the nightly roachtests don't even have the crdb_test build tag on, so by hiding it behind the tag we are basically trimming our chances of catching these problems before they reach production. We would really need a suite of roachtests that runs the debug build.
Let's please discuss the tangent on the related issue and focus here on the review, as most of the context here will be lost anyway.

tbg · 2023-01-10T10:08:21Z

Also filed #94986 for the general issue of making it easier to write assertions.

tbg · 2023-01-13T10:29:50Z

Friendly ping.

nvanbenschoten

Reviewed 3 of 4 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @pavelkalinnikov and @tbg)

pkg/kv/kvserver/apply/task.go line 284 at r1 (raw file):

					return rejectErr
				}
				return err

Don't we still want to return the error regardless of its type?

Also, this now "leaks" commands in the non-err removed case. Is that assumption that all other cases are fatal to the process, so we don't care about leaking commands in those cases?

pkg/kv/kvserver/replica_test.go line 8134 at r1 (raw file):

	proposalDoneCh := proposal.doneCh

	g, _, pErr := repl.concMgr.SequenceReq(ctx, nil /* guard */, concurrency.Request{

For the sake of readers, it seems more appropriate to sequence the request before the call to requestToProposal, because that's the real order of operations.

The conjecture in cockroachdb#86547 is that a finished proposal somehow makes its way into the proposal map, most likely by never being removed prior to being finished. This commit adds an assertion that we're never outright *inserting* a finished proposals, and better documents the places in which we're running a risk of violating the invariant. It also clarifies the handling of proposals in an apply batch when a replication change that removes the replica is encountered. I suspected that this could lead to a case in which proposals would be finished despite remaining in the proposals map. Upon inspection this turned out to be incorrect - the map (at least by code inspection) is empty at that point, so the invariant holds trivially. Unfortunately, that leaves me without an explanation for cockroachdb#86547, but the newly added invariants may prove helpful. Touches cockroachdb#86547.

…osals map

tbg

Updated, RFAL.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten and @pavelkalinnikov)

pkg/kv/kvserver/apply/task.go line 284 at r1 (raw file):
Good catch - the error should've been returned either way, glad you caught this.

Also, this now "leaks" commands in the non-err removed case. Is that assumption that all other cases are fatal to the process, so we don't care about leaking commands in those cases?

Yes, errors from the apply loop are fatal. We may handle them in the future by calling into some version of replica removal at the caller, and replica removal would be in charge of releasing all pending commands.

nvanbenschoten

Reviewed 3 of 3 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @pavelkalinnikov)

tbg · 2023-02-08T21:09:59Z

bors r=nvanbenschoten
TFTR!

craig · 2023-02-08T22:08:30Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2023-02-08T22:11:27Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2023-02-09T00:23:37Z

Build failed:

Bazel Essential CI (Cockroach)

tbg · 2023-02-09T07:55:31Z

bors r=nvanbenschoten

craig · 2023-02-09T08:56:58Z

Build succeeded:

Bazel Essential CI (Cockroach)

… in proposals map" Reverts cockroachdb#94825. This reverts commit ac23f46.

…posals map" Reverts cockroachdb#94825. This reverts commit 15b1c6a.

98481: kvserver: revert recent changes to reproposals r=pavelkalinnikov a=tbg Reverts #97606, #97564, #94825, #94633. - Revert "kvserver: disable assertion 'finished proposal inserted'" - Revert "kvserver: narrow down 'finishing a proposal with outstanding reproposal'" - Revert "kvserver: fill gaps in comment near tryReproposeWithNewLeaseIndex" - Revert "kvserver: hoist early return out of tryReproposeWithNewLeaseIndex" - Revert "fixup! kvserver: prevent finished proposal from being present in proposals map" - Revert "kvserver: prevent finished proposal from being present in proposals map" - Revert "kvserver: improve reproposal assertions and documentation" Closes #97973. Epic: CRDB-25287 Release Note: none 98537: sql: check row level ttl change before truncating a table r=chengxiong-ruan a=chengxiong-ruan Fixes: #93443 Release note (sql change): This commit fixed a bug where crdb paniced wehn user tried to truncate a table which is has an ongoing row level ttl change. We still don't support table truncates in this scenario, but a more gentle unimplemented error is returned instead of panic. 98575: cdc: use int64 for emitted bytes telemetry r=miretskiy a=jayshrivastava Previously, the stored `emitted_bytes` field was an int32, which can hold a maximum value of 2.1GB. This value is too small because the logging period is 24h and changefeeds can emit much more than 2.1GB in 24h. This change updates the field to be an int64, which solves this problem. Epic: None Release note: None 98582: ci: allow-list `BUILD_VCS_NUMBER` env var in cloud unit tests r=jlinder a=rickystewart This job was filing issues linking to the wrong commit. Epic: none Release note: None Co-authored-by: Tobias Grieger <tobias.b.grieger@gmail.com> Co-authored-by: Chengxiong Ruan <chengxiongruan@gmail.com> Co-authored-by: Jayant Shrivastava <jayants@cockroachlabs.com> Co-authored-by: Ricky Stewart <rickybstewart@gmail.com>

tbg marked this pull request as ready for review January 9, 2023 10:30

tbg requested a review from a team January 9, 2023 10:30

tbg requested a review from a team as a code owner January 9, 2023 10:30

tbg requested review from nvanbenschoten and pav-kv January 9, 2023 10:31

tbg mentioned this pull request Jan 10, 2023

kvserver: audit fatal assertions and downgrade some to test builds only #94979

Open

tbg force-pushed the finish-application branch from 28856d7 to c22b5af Compare January 10, 2023 07:41

msbutler mentioned this pull request Jan 24, 2023

kvserver: v22.1.4: nil pointer in poisonInflightLatches #86547

Closed

nvanbenschoten reviewed Jan 30, 2023

View reviewed changes

tbg force-pushed the finish-application branch from c22b5af to 15b1c6a Compare February 6, 2023 07:31

fixup! kvserver: prevent finished proposal from being present in prop…

ac23f46

…osals map

tbg commented Feb 6, 2023

View reviewed changes

tbg requested a review from nvanbenschoten February 6, 2023 07:39

nvanbenschoten approved these changes Feb 8, 2023

View reviewed changes

tbg mentioned this pull request Feb 9, 2023

TestDockerCLI/test_demo_partitioning.tcl failed #96797

Closed

craig bot merged commit 95fcd95 into cockroachdb:master Feb 9, 2023

tbg deleted the finish-application branch February 9, 2023 10:17

tbg mentioned this pull request Mar 3, 2023

kvserver: revert recent reproposal changes #97973

Closed

5 tasks

tbg added a commit to tbg/cockroach that referenced this pull request Mar 13, 2023

Revert "fixup! kvserver: prevent finished proposal from being present…

500bc82

… in proposals map" Reverts cockroachdb#94825. This reverts commit ac23f46.

tbg added a commit to tbg/cockroach that referenced this pull request Mar 13, 2023

Revert "kvserver: prevent finished proposal from being present in pro…

e23a2ea

…posals map" Reverts cockroachdb#94825. This reverts commit 15b1c6a.

tbg mentioned this pull request Mar 13, 2023

kvserver: revert recent changes to reproposals #98481

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kvserver: prevent finished proposal from being present in proposals map #94825

kvserver: prevent finished proposal from being present in proposals map #94825

tbg commented Jan 6, 2023 •

edited

Loading

cockroach-teamcity commented Jan 6, 2023

erikgrinaker commented Jan 9, 2023

tbg commented Jan 10, 2023

tbg commented Jan 10, 2023

pav-kv commented Jan 10, 2023 •

edited

Loading

tbg commented Jan 10, 2023

tbg commented Jan 10, 2023

tbg commented Jan 13, 2023

nvanbenschoten left a comment

tbg left a comment

nvanbenschoten left a comment

tbg commented Feb 8, 2023

craig bot commented Feb 8, 2023

craig bot commented Feb 8, 2023

craig bot commented Feb 9, 2023

tbg commented Feb 9, 2023

craig bot commented Feb 9, 2023

kvserver: prevent finished proposal from being present in proposals map #94825

kvserver: prevent finished proposal from being present in proposals map #94825

Conversation

tbg commented Jan 6, 2023 • edited Loading

cockroach-teamcity commented Jan 6, 2023

erikgrinaker commented Jan 9, 2023

tbg commented Jan 10, 2023

tbg commented Jan 10, 2023

pav-kv commented Jan 10, 2023 • edited Loading

tbg commented Jan 10, 2023

tbg commented Jan 10, 2023

tbg commented Jan 13, 2023

nvanbenschoten left a comment

Choose a reason for hiding this comment

tbg left a comment

Choose a reason for hiding this comment

nvanbenschoten left a comment

Choose a reason for hiding this comment

tbg commented Feb 8, 2023

craig bot commented Feb 8, 2023

craig bot commented Feb 8, 2023

craig bot commented Feb 9, 2023

tbg commented Feb 9, 2023

craig bot commented Feb 9, 2023

tbg commented Jan 6, 2023 •

edited

Loading

pav-kv commented Jan 10, 2023 •

edited

Loading